Fixed `displayError` being incorrectly used for all tests attempts #530

Andarist · 2024-06-13T12:33:22Z

This PR addresses one of the issues recognized in https://linear.app/replay/issue/PRO-597

changeset-bot · 2024-06-13T12:33:26Z

🦋 Changeset detected

Latest commit: cc5e1ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@replayio/cypress	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

socket-security · 2024-06-13T12:34:00Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@cypress/[email protected]	environment, filesystem, network	`+22`	1.2 MB	cypress-npm-publisher
npm/[email protected]	environment, filesystem, shell, unsafe Transitive: eval	`+41`	9.54 MB	atofstryker

🚮 Removed packages: npm/@cypress/[email protected], npm/[email protected]

View full report↗︎

socket-security · 2024-06-13T12:34:03Z

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert	Package	Note	Source
Install scripts	npm/[email protected]	Install script: postinstall Source: `node index.js --exec install`	`packages/cypress/example/package.json` `packages/cypress/package.json`
Install scripts	npm/[email protected]	Install script: postinstall Source: `node index.js --exec install`	Source

View full report↗︎

Next steps

What is an install script?

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@SocketSecurity ignore npm/[email protected]
@SocketSecurity ignore npm/[email protected]

replay-io · 2024-06-13T12:36:55Z

Status	Complete ↗︎
Commit	`2c19c3c`
Results	❌ 2 Failed clicks a disappearing button should fail on this test ✅ 42 Passed adds items adds new items using a custom command adds todos following the fixture adds todos following the fixture adds todos following the fixture adds todos following the fixture calls inform complete all checkbox should update state when items are completed / cleared gets a number only gets a number only gets a number should allow me to add todo items should allow me to clear the complete state of all items should allow me to display active items should allow me to display all items should allow me to display completed items should allow me to edit an item should allow me to mark all items as completed should allow me to mark items as complete should allow me to un-mark items as complete should append new items to the bottom of the list should be hidden when there are no items that are completed should cancel edits on escape should clear text input field when an item is added should display the correct text should display the current number of todo items should focus on the todo input field should hide #main and #footer should hide other controls when editing should highlight the currently applied filter should intercept postman should invoke some commands that have exceptional option handling should log should persist its data should remove completed items when clicked should remove the item if an empty text string was entered should respect the back button should save edits on blur should show #main and #footer when items added should trim entered text should trim text input yields a number

Andarist · 2024-06-13T12:37:15Z

packages/cypress/package.json

@@ -54,6 +54,6 @@
    "ws": "^8.14.2"
  },
  "peerDependencies": {
-    "cypress": ">=5.3.0"
+    "cypress": "^13"


Cypress 13 was released almost one year ago (08/29/2023). We don't have any tests for this whole integration, trying to test compatibility manually with an extended range of versions is a nightmare. I don't feel we can quite guarantee compatibility right now with other versions, it's impractical.

If requested by clients we can always loosen up this range after manually confirming that a particular combination works. We'll release a new major of this package soon so I think we should grab the opportunity and bump this here.

Note that supporting more right now would require some extra work here, I changed this because I ran into typing problems (that likely were related to breaking changes in Cypress).

I support this.

Andarist · 2024-06-13T12:38:29Z

packages/cypress/src/steps.ts

-      // Cypress 10.9 types are wrong here ... duration doesn't exist but wallClockDuration does
-      approximateDuration: a.duration || (a as any).wallClockDuration || 0,
+      // those properties don't exist since Cypress 13: https://github.com/cypress-io/cypress/pull/27230
+      approximateDuration: (a as any).duration || (a as any).wallClockDuration || 0,


With the latest version of Cypress this is non-sensical. It will just always be 0. I'm afraid to remove it though, without confirming that devtools and dashboard would be fine without this

Can we add a Linear task + TODO comment to follow up here? Else it will get forgotten about.

done, PRO-640

Andarist · 2024-06-13T12:40:37Z

packages/cypress/src/steps.ts

+      // `.displayError` represents the error reported by the last attempt
+      // for all of the previous attempts we rely on the search for the last errored step in `groupStepsByTest`
+      // if an error is found there, it will be set on the test, the information set here is not overriden though


This logic is a little bit wonky but it makes sense. The .displayError is something computed by Cypress so it should have higher accuracy than what we can compute on our own. OTOH, we have to compute this on our own for non-last attempts because Cypress doesn't expose this information to us.

It would be great to refactor this so we don't have to rely on a mutation elsewhere but that's out of this PR's scope

Andarist · 2024-06-13T12:42:41Z

packages/cypress/src/support.ts

@@ -579,7 +579,7 @@ function register() {
          logs: () => [],
          add: () => {},
          get: (key?: any) => (!key ? log : log[key]),
-          toJSON: () => log,
+          toJSON: () => [],


the previous version was completely crashing Cypress, the toJSON's type here expects string[] so I used an empty array to ensure that it's compatible. In Cypress 10 log was typed as any and maybe at that time it worked OK, dunno. Today it's an object so we can see how it isn't compatible with the expected string[].

I verified that we can still render cy.log just fine with this. It's quite plausible that we just don't rely on the return value of this function.

Andarist requested review from bvaughn and hbenl June 13, 2024 12:33

Andarist commented Jun 13, 2024

View reviewed changes

hbenl approved these changes Jun 13, 2024

View reviewed changes

Base automatically changed from remove-cli-from-cypress-pkg to main June 13, 2024 14:50

Fixed displayError being incorrectly used for all tests attempts

14d64ba

Andarist force-pushed the fix/wrong-reuse-of-displayerror branch from f866693 to 14d64ba Compare June 13, 2024 14:52

Andarist added 2 commits June 13, 2024 16:54

add todo comment

4b0a09a

add changesets

cc5e1ac

Andarist merged commit 81179b9 into main Jun 13, 2024
6 of 7 checks passed

Andarist deleted the fix/wrong-reuse-of-displayerror branch June 13, 2024 15:05

github-actions bot mentioned this pull request Jun 13, 2024

Version Packages #521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed `displayError` being incorrectly used for all tests attempts #530

Fixed `displayError` being incorrectly used for all tests attempts #530

Andarist commented Jun 13, 2024

changeset-bot bot commented Jun 13, 2024 •

edited

Loading

socket-security bot commented Jun 13, 2024 •

edited

Loading

socket-security bot commented Jun 13, 2024 •

edited

Loading

replay-io bot commented Jun 13, 2024 •

edited

Loading

Andarist Jun 13, 2024

bvaughn Jun 13, 2024

Andarist Jun 13, 2024

bvaughn Jun 13, 2024

Andarist Jun 13, 2024

Andarist Jun 13, 2024

Andarist Jun 13, 2024

Fixed displayError being incorrectly used for all tests attempts #530

Fixed displayError being incorrectly used for all tests attempts #530

Conversation

Andarist commented Jun 13, 2024

changeset-bot bot commented Jun 13, 2024 • edited Loading

🦋 Changeset detected

socket-security bot commented Jun 13, 2024 • edited Loading

socket-security bot commented Jun 13, 2024 • edited Loading

Next steps

replay-io bot commented Jun 13, 2024 • edited Loading

Andarist Jun 13, 2024

Choose a reason for hiding this comment

bvaughn Jun 13, 2024

Choose a reason for hiding this comment

Andarist Jun 13, 2024

Choose a reason for hiding this comment

bvaughn Jun 13, 2024

Choose a reason for hiding this comment

Andarist Jun 13, 2024

Choose a reason for hiding this comment

Andarist Jun 13, 2024

Choose a reason for hiding this comment

Andarist Jun 13, 2024

Choose a reason for hiding this comment

Fixed `displayError` being incorrectly used for all tests attempts #530

Fixed `displayError` being incorrectly used for all tests attempts #530

changeset-bot bot commented Jun 13, 2024 •

edited

Loading

socket-security bot commented Jun 13, 2024 •

edited

Loading

socket-security bot commented Jun 13, 2024 •

edited

Loading

replay-io bot commented Jun 13, 2024 •

edited

Loading